-
Notifications
You must be signed in to change notification settings - Fork 727
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove synchronization in Reference and unused methods in ReferenceQueue #20694
Conversation
jenkins test sanity plinux jdk8 |
The successful PR build https://openj9-jenkins.osuosl.org/job/PullRequest-OpenJ9/6578/ |
The deadlock happens because the two locks are acquired in different order in the two code paths.
Removing the |
The correct fix is to avoid the different order in which the two locks are acquired. Example: In
@dmitripivkine Does it matter if we move the dequeue operation at the end? Also, is there a requirement to |
Because this is a public class, we don't have complete control over the synchronization order. Removing synchronization in All of the uses of |
Once the |
I don't think the change to remove() is correct in #20694 (comment) I've pushed a new commit (I can squash before merge) to ensure lock ordering and consistency between enqueue and dequeue. |
|
jenkins test sanity plinux jdk8 |
jcl/src/java.base/share/classes/java/lang/ref/ReferenceQueue.java
Outdated
Show resolved
Hide resolved
jcl/src/java.base/share/classes/java/lang/ref/ReferenceQueue.java
Outdated
Show resolved
Hide resolved
Previous PR build https://openj9-jenkins.osuosl.org/job/PullRequest-OpenJ9/6597/ passed. |
jenkins compile plinux jdk8 |
jenkins compile plinux jdk11 |
Just some known problems in the test results, the non-network failures passed in reruns. |
7ffa7c4
to
2ecb6c9
Compare
jenkins compile plinux jdk8,jdk21 |
jcl/src/java.base/share/classes/java/lang/ref/ReferenceQueue.java
Outdated
Show resolved
Hide resolved
|
jenkins compile amac jdk21 |
Created ibmruntimes/openj9-openjdk-jdk#900 and a backport for jdk23. I accidentally pushed the same change to jdk21. We shouldn't remove the constructor until z/OS and valhalla are also updated. |
Issue eclipse-openj9#13823 Signed-off-by: Peter Shipton <[email protected]>
Signed-off-by: Peter Shipton <[email protected]>
@pshipton Is the testing from #20694 (comment) still good? or a new set of testing needs to be run for the latest changes? |
I think the testing is still valid. I restored some missing return statements, but nothing failed as a result of them missing, I don't expect any failure from adding them back. Otherwise the changes are cosmetic. |
jenkins compile plinux jdk8,jdk11 |
Issue #13823